Skip to content

Conversation

GNL10
Copy link
Contributor

@GNL10 GNL10 commented Sep 2, 2025

No description provided.

@GNL10
Copy link
Contributor Author

GNL10 commented Sep 2, 2025

I made sure that I covered all the usages of these constructors by forcing compilation errors (normal compilation, compiling the UTs as well).
If there are pieces of code that could use these constructors but are not getting compiled with these commands (and are not being tested), I probably missed them.

Copy link

sonarqubecloud bot commented Sep 3, 2025

Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really see the advantage with this reordering. It looks rather dangerous. I don't understand how you made sure that all ids and messages are reordered - you said something about a compiler error.

I appreciate you try to fix something here.. would you like some suggestions for bugs that you can look at?

@GNL10
Copy link
Contributor Author

GNL10 commented Sep 11, 2025

I was just fixing the TODO that is in the code. The main advantage is simply to give consistency to the order of the msg and id fields in the multiple constructors of ErrorMessage.
After this MR they will all have the same order.

Regarding the compiler error, I was just mentioning that in order to find all applicable instances of the constructor, I forced a compiler error to happen specifically in these constructors that I updated. Then I updated the parameters in all of them and then I reverted the change that triggered the compiler error.

Sure I can take suggestions for bugs to look at!

@danmar
Copy link
Owner

danmar commented Sep 23, 2025

sorry for slow reply. I was at cppcon last week and was extra busy before it..

Sure I can take suggestions for bugs to look at!

Thanks! I have a suggestion below..

@danmar
Copy link
Owner

danmar commented Sep 24, 2025

Here is a suggestion. I used AI to look for logic errors in lib/cppcheck.cpp and it had the complaints listed below. After a quick look I think the complaints seem correct:

  1. npos in string operations (Line 326-328)

if (str[startPos] == '"') {
const std::string::size_type endPos = str.find('"', startPos + 1);
ret.push_back(str.substr(startPos + 1, endPos - startPos - 1));
Missing check for endPos == std::string::npos before using it in arithmetic and substr().

  1. substr (Line 1002)

filename2 = mSettings.plistOutput + filename2.substr(0, filename2.find('.')) + "_" + std::to_string(fileNameHash) + ".plist";
If filename2.find('.') returns npos, the substr uses it as length parameter without validation.

  1. Missing npos check before arithmetic (Line 1997-1998)

const std::size_t endLinePos = line.rfind(':', endColumnPos-1);
const std::size_t endNamePos = line.rfind(':', endLinePos - 1);
If endColumnPos is npos, then endColumnPos-1 causes integer underflow.

  1. Suspicious logic inversion (Line 1899)

if (mSettings.severity.isEnabled(Severity::information) && file.empty())
return;
This skips reporting when information is enabled, which seems backwards.

  1. Message duplication bug (Line 1873-1876)

if (numberOfConfigurations > mSettings.maxConfigs)
msg << " of " << numberOfConfigurations << " configurations. Use --force to check all configurations.\n";
if (file.empty())
msg << " configurations. Use --force to check all configurations. For more details, use --enable=information.\n";
Both conditions can be true, causing duplicate text. Should be else if.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants